Hello @ArtyomKaltovich! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file pandas/tests/scalar/timestamp/test_timezones.py:

Line 446:89: E501 line too long (119 > 88 characters)

Some tests failed due to changes, mostly pickle ones.
I think that tz should be key only parameter (btw, usually it is used in such way), but it causes failures.
What should I do? Change or not to change?)

#31930 is more about raising when illegal combinations of arguments are specified together (not just tz)

I think there needs to be more discussion on the issue before we move forward with a solution.

Hello @mroeschke
Thank you for your feedback.

I've just faced with this bug recently at my job and decided to fix it. But you are right, it is more connected to the issue #31929

What if the other prohibited combinations? I can name only this one and ts_input with year, month and day specified via key params. I can add this check if it is not yet presented. Are there any more cases? functools.singledispatch could help us here, but it will probably slow down the execution. So I guess there is no better way here except the current one (I mean isinstance checking it the constructor body). Don't you agree?

If you need a little bit more time to think about refactoring, can we for now submit it as a fix for #31929 ? Because, as I said previously it prevents me to implement work tasks and it is unexpected behavior what is good to avoid.

I've just need to know if changing tz to key only param is acceptable or not?

Thank you and sorry for mentioning the wrong issue :)

Regards,
Artyom.

I proposed #31930 (comment) as the only legal combination of parameters for #31930. Probably needs more discussion on that front.

As for #31929 which you are trying to address, rearranging the arguments in the constructor would be an API change and would need to undergo a deprecation cycle. The easier solution in my option would be to deprecate the tz argument entirely (which would still require a deprecation cycle)

Ok, I see your point.

So for now, I can try to bypass the bug without changing api. That will fix the problem and #31929 could be closed, while we can still think about better approach for #31930

Actually singledispatch works with functions with different amount of parameters.

from datetime import datetime
from functools import singledispatch


@singledispatch
def Timestamp(tsinput):
    raise NotImplementedError('Unsupported type')


@Timestamp.register(int)
def _(year, month, day):
    print(f"Timestamp({year}, {month}, {day})")


@Timestamp.register(str)
def _(ts_info):
    print(f"Timestamp({ts_info})")


@Timestamp.register(datetime)
def _(ts_info):
    print(f"Timestamp({ts_info})")

if __name__ == '__main__':
    Timestamp(2020, 2, 2)  # if omit the last arg  
                           # TypeError: _() missing 1 required positional argument: 'day'
                           # will be generated.
    Timestamp("2019-11-11")
    Timestamp(datetime(2017, 3, 4))

produces

Timestamp(2020, 2, 2)
Timestamp(2019-11-11)
Timestamp(2017-03-04 00:00:00)

but of course more research of performance, function wrupping and doc generation are needed.

Do you want to try to address #31930 with singledispatch in this PR @ArtyomKaltovich?

Do you want to try to address #31930 with singledispatch in this PR @ArtyomKaltovich?

If it is acceptable variant, why not?)

But I am not familiar with Pandas CI performance testing, where can I read/know about it?

i don't think we can use singledispatch in cython (well we can but its going to be non-performant); you can try to prove me wrong though.

In any event, we simply want to validate combinations of parameters and reject invalid ones.

Hello @jreback
Thank you for your feedback.
I am also afraid of performance issue, but maybe singledispatch create the same code with isinstance as already presented one. That's why I've asked how can I measure it with Pandas performance testing?

we cannot change the position of these args
Thank you I will revert parameter order. I can submit a fix for #31929 independently while we a thinking about better parameter validation approach.

Hello @mroeschke @jreback
I was trying to leave param order as is. The following code works:

    stamp = Timestamp(2019, 1, 2, 3, 4, 5, 6, 7, tzinfo=pytz.timezone("UTC"))
    stamp = Timestamp(year=2019, month=1, day=2, hour=3, minute=4, second=5, microsecond=6, nanosecond=7, tz="UTC")

but it still has problems with such code:

        stamp = Timestamp(2019, 1, 1, tz="EST5EDT")
        E   TypeError: __new__() got multiple values for keyword argument 'tz'

Because tz is the third arg and 1 is passed as tz, not day.

I can create PR, because it fixes at least one problem, or we can continue to experiment, but I do not see any ways to fix it except singledispatch or parameter reordering :(

Regards.

In terms of performance testing see: https://pandas.pydata.org/pandas-docs/stable/development/contributing.html#running-the-performance-test-suite

I think these issues can be solved together by either reordering parameters or using singledispatch

Thank you @mroeschke
I will investigate it.

@ArtyomKaltovich do you have time to merge in master and address the review comments?

Hello @mroeschke
Unfortunately I didn't have enough time to work on it due to work and study, I am planing to do it on this week.

Regards.

closing as stale
if u want to continue pls ping